[integration_test] stabilize multi-location lazy prune test - #220
[integration_test] stabilize multi-location lazy prune test#220wangxiyu191 wants to merge 4 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a45585738
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._finish_write_blocks(write_session_id, len(locations), | ||
| success=False) |
There was a problem hiding this comment.
When a retry returns only a partial set of locations, this success=False finish queues an asynchronous delete for those newly allocated locations. In this test the dummy backend URI is deterministic for the same instance/spec/key, so a later retry can allocate and touch the same file path before the previous failed-attempt delete runs; that stale delete can then remove the freshly committed GroupA data and make the final tp0 query flaky. Please wait for the failed location delete to complete, or otherwise avoid retrying on the same deterministic URIs until the abort cleanup is done.
Useful? React with 👍 / 👎.
| if len(locations) == expected_location_count: | ||
| self._touch_cache_locations(locations) | ||
| self._finish_write_blocks(write_session_id, len(locations)) |
There was a problem hiding this comment.
If _wait_for_prefix_specs returns right after GetCacheLocation filtered the deleted tp0 files, the stale-location prune is still only queued, so this first StartWriteCache can allocate all three GroupA locations before that queued task CASes the old locations. Because the dummy backend reuses the same URI for the same instance/spec/key, accepting this full allocation immediately can let the old prune task delete the files just touched here, making the final tp0 query flaky. Please wait for the stale prune's storage delete/metadata removal to finish before treating a full allocation as safe.
Useful? React with 👍 / 👎.
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The latest commit (be60bba) resolves both race conditions flagged by the earlier Codex review:
- Aborted retry cleanup:
_touch_cache_locations(locations)+_wait_for_locations_absent(locations)now ensures a failed attempt's files are fully deleted before the next retry, preventing a stale abort-delete from removing freshly committed data on the same deterministic URI. - Stale prune vs. rewrite race: The marker-file approach (
_touch_cache_locations(locs_a)before rewrite) is a clean barrier — if metadata still exists,StartWriteCachetreats GroupA as covered and the retry aborts; once the queued prune deletes both the markers and metadata, allocation proceeds. The_verify_block_keysassertion is correct for the dummy backend's hex file naming (Uint64ToHexwithkey_count_per_file=1).
Minor Observations
- Unused parameter:
expected_location_countin_write_blocks_with_groupis never passed by any caller (all call sites use the defaultNone). The retry path uses the separate_write_blocks_with_group_retrymethod instead. This looks like vestigial code from an earlier iteration and could be removed for clarity. - Inconsistent sleep on last poll iteration:
_wait_for_prefix_specscallstime.sleepunconditionally on every iteration, including the final one beforeself.fail. The other two polling helpers (_write_blocks_with_group_retry,_wait_for_locations_absent) correctly guard withif attempt < self.POLL_ATTEMPTS. Trivial — just wastes ~1 second on failure.
Out of Scope
test_all_locations_lost_breaks_prefix still uses a fixed time.sleep(2) for prune settling and could benefit from the same condition-driven polling approach if it ever becomes flaky.
🤖 Generated by Qoder
1 participant
Summary
multi_location_testaround lazy stale-location pruning by polling until the read path is tp1-only, then retryingStartWriteCache(GroupA)until all missing GroupA locations are allocated.FinishWriteCacheall false, and wait for the aborted attempt's files to be deleted before retrying.Root Cause
GetCacheLocationfilters stale locations from the read result and submits metadata pruning asynchronously. The test treated a tp1-only query result as proof that metadata pruning had completed, so an immediateStartWriteCache(GroupA)could race with the async deletion and return a partialblock_mask.Review Follow-up
The fixed settle window was removed. The rewrite phase now uses condition-driven barriers:
StartWriteCachetreat GroupA as covered until the queued prune deletes the markers and removes metadata; if prune already finished, the markers become the rewritten data files.Validation
python -m py_compile integration_test/reclaimer/multi_location_test.pygit diff --checkbazelisk test //integration_test/reclaimer:multi_location_test --cache_test_results=no --test_output=errors